Repository navigation
PiPNN 4/6: integrate disk build pipeline - #1291
weiyaoluo (SeliMeli) wants to merge 67 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates PiPNN graph construction into the DiskANN disk-index build pipeline and exposes it through the benchmark CLI inputs, including automated fixture-based tests to validate the new JSON configuration and execution paths.
Changes:
- Add a
BuildAlgorithmselector (Vamana vs PiPNN) with JSON-facing PiPNN parameters, plus memory-estimate-based fallback to Vamana for disk builds. - Implement PiPNN graph building adapters for disk builds and in-memory benchmark builds, including vector-store “flat prefix” access for dense layouts.
- Add a shared cached CLI fixture runner and new PiPNN benchmark fixtures (disk + in-memory) to validate end-to-end CLI wiring.
Reviewed changes
Copilot reviewed 33 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-providers/src/utils/rayon_util.rs | Exposes underlying Rayon pool reference for APIs that need &rayon::ThreadPool. |
| diskann-providers/src/storage/mod.rs | Re-exports new adjacency-graph save helper. |
| diskann-providers/src/storage/bin.rs | Adds save_adjacency_graph for canonical adjacency graph serialization. |
| diskann-providers/src/model/graph/provider/async_/fast_memory_vector_provider.rs | Exposes unsafe dense-prefix slice access to support PiPNN build path. |
| diskann-providers/src/model/graph/provider/async_/common.rs | Implements AlignedMemoryVectorStore::flat_prefix and adds a contiguous-prefix test. |
| diskann-disk/src/lib.rs | Re-exports PiPNN parameters (feature-gated) and exposes build-algorithm selection. |
| diskann-disk/src/build/mod.rs | Re-exports BuildAlgorithm and (feature-gated) PiPNN parameters. |
| diskann-disk/src/build/configuration/mod.rs | Introduces build-algorithm configuration module and re-exports its types. |
| diskann-disk/src/build/configuration/disk_index_build_parameter.rs | Adds build_algorithm, PiPNN constructor, PiPNN config conversion, and memory-estimate-based fallback. |
| diskann-disk/src/build/configuration/build_algorithm.rs | New: serde-driven BuildAlgorithm enum and PiPNN JSON parameter struct. |
| diskann-disk/src/build/builder/build/pipnn.rs | New: PiPNN graph build adapter producing a canonical on-disk adjacency graph. |
| diskann-disk/src/build/builder/build/pipnn/tests.rs | New: disk-build integration tests for PiPNN selection, fallback, and header correctness. |
| diskann-disk/src/build/builder/build.rs | Routes graph construction to PiPNN (when selected) while keeping existing Vamana pipeline as default. |
| diskann-disk/Cargo.toml | Adds optional diskann-pipnn dependency + feature, plus test deps for new tests. |
| diskann-benchmark/src/main.rs | Refactors registry creation and adds PiPNN fixture test hook (feature-gated). |
| diskann-benchmark/src/inputs/graph_index.rs | Adds PiPNN build-algorithm selection for graph-index build inputs (feature-gated). |
| diskann-benchmark/src/inputs/disk.rs | Adds alpha, makes quantization_type optional, and adds disk build-algorithm selection + validation. |
| diskann-benchmark/src/index/build.rs | Adds PiPNN in-memory build path leveraging dense flat-prefix when possible. |
| diskann-benchmark/src/index/benchmarks.rs | Wires PiPNN build path selection into benchmark build flow. |
| diskann-benchmark/src/disk_index/build.rs | Wires disk BuildAlgorithm into disk-index build parameter construction. |
| diskann-benchmark/Cargo.toml | Adds optional PiPNN dependency/feature and enables fixture runner for dev-tests. |
| diskann-benchmark/tests/pipnn/inmemory/stdout.txt | New: expected output for in-memory PiPNN CLI fixture. |
| diskann-benchmark/tests/pipnn/inmemory/stdin.txt | New: commands for in-memory PiPNN CLI fixture. |
| diskann-benchmark/tests/pipnn/inmemory/input.json | New: PiPNN in-memory input JSON fixture. |
| diskann-benchmark/tests/pipnn/disk/stdout.txt | New: expected output for disk PiPNN CLI fixture. |
| diskann-benchmark/tests/pipnn/disk/stdin.txt | New: commands for disk PiPNN CLI fixture. |
| diskann-benchmark/tests/pipnn/disk/input.json | New: PiPNN disk input JSON fixture. |
| diskann-benchmark-runner/src/lib.rs | Exposes new fixture runner module for tests/downstream crates (feature-gated). |
| diskann-benchmark-runner/src/fixture.rs | New: shared cached CLI fixture runner implementation. |
| diskann-benchmark-runner/src/fixture/tests.rs | New: unit tests for fixture runner behavior. |
| diskann-benchmark-runner/src/app.rs | Updates integration-test documentation and switches tests to the shared fixture runner. |
| diskann-benchmark-runner/Cargo.toml | Adds test-fixtures feature + optional tempfile dep for downstream fixture usage. |
| Cargo.lock | Adds/records diskann-pipnn and serde_json dependency changes. |
| .cargo/mutants.toml | Excludes new fixture overwrite behavior from mutation testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8134eb3 to
ce5f054
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
diskann-disk/src/build/configuration/disk_index_build_parameter.rs:17
estimate_pipnn_peak_memoryusessize_of::<...>()unqualified, but this module doesn’t importstd::mem::size_of. This will fail to compile under the repo’s Rust toolchain.
use std::num::NonZeroUsize;
use diskann::ANNError;
#[cfg(feature = "pipnn")]
use diskann::ANNResult;
use thiserror::Error;
#[cfg(feature = "pipnn")]
use super::PiPNNParameters;
use super::{BuildAlgorithm, QuantizationType};
diskann-disk/src/build/configuration/disk_index_build_parameter.rs:113
DiskIndexBuildParametersis publicly re-exported (viadiskann-disk’s public API) and this change removesCopyfrom its derives. That is a breaking API change for downstream users that relied on implicit copies; if this is intentional, it should be called out explicitly (and potentially gated or versioned accordingly).
/// Parameters specific for disk index construction.
#[derive(Clone, PartialEq, Debug)]
pub struct DiskIndexBuildParameters {
ce5f054 to
3893460
Compare
3893460 to
f836f69
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
diskann-disk/src/build/configuration/disk_index_build_parameter.rs:204
disk_index_build_parameter.rsenables#![warn(missing_docs)], and the newpub(crate) fn pipnn_confighas no doc comment. If CI promotes warnings to errors, this will fail builds; even if not, it adds new lint noise.
#[cfg(feature = "pipnn")]
pub(crate) fn pipnn_config(&self) -> Option<diskann_pipnn::PiPNNConfig> {
match &self.build_algorithm {
BuildAlgorithm::PiPNN(config) => Some(config.into()),
BuildAlgorithm::Vamana => None,
}
}
diskann-disk/src/build/builder/build/pipnn.rs:49
pipnn::build_graphreads the full dataset into memory (read_bin) and then callsfind_medoid_with_sampling, which performs another full pass over the dataset viaVectorDataIterator(plus a sampled centroid pass). On large datasets this doubles I/O and parsing work even though the vectors are already in memory.
let data =
read_bin::<Data::VectorDataType>(&mut builder.storage_provider.open_reader(&data_path)?)?;
let context = PiPNNBuildContext::new(
config,
&builder.index_configuration.config,
diskann-providers/src/storage/bin.rs:388
save_adjacency_graphis re-exported as a public API, but its signature exposesdiskann::graph::AdjacencyList<u32>, forcing downstream callers to take a dependency ondiskanngraph types just to usediskann-providersserialization. If the goal is a narrow cross-crate adjacency entry point, consider changing this API to accept adjacency rows as plain slices (or any type that derefs/borrows to&[u32]) to avoid leakingdiskanninternals intodiskann-providers’ public surface.
pub fn save_adjacency_graph<P>(
adjacency: &[AdjacencyList<u32>],
max_degree: u32,
provider: &P,
start_point: u32,
f836f69 to
2181da2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pipnn-stack/03-core #1291 +/- ##
=====================================================
Coverage 91.72% 91.72%
=====================================================
Files 572 574 +2
Lines 114177 114524 +347
=====================================================
+ Hits 104728 105051 +323
- Misses 9449 9473 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2181da2 to
19c3b5a
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Match the default configuration documented on PiPNNConfig and point the JSON parameters at those docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The disk test fixture takes optional PiPNN parameters. The new test builds the 256-point MinMax sample with PiPNN and checks exact search results, which failed with a length error before PiPNN sized its f32 buffers with the converted row length. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PiPNNParameters gets max_leaders, default 1000, and passes it to PiPNNConfig. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…update/04-integration
The PiPNN build context now takes the f32 row length. IndexConfiguration::dim is the stored length, which differs for MinMax, so the adapter reads the decoded length from the first row, as the PQ generator does. The PiPNN parameters gain max_depth with default 10. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…date/04-integration
…date/04-integration
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…date/04-integration
Small c_max values need more than 10 split levels on normal data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…date/04-integration
Purpose
This PR connects PiPNN graph construction to the production disk-index builder.
It uses the existing DiskANN graph format. It does not add a PiPNN-specific index format.
Main changes
BuildAlgorithmaccepts explicit PiPNN parameters.diskann::graph::pipnn::build_graph.The PiPNN core returns rows for real points only. The disk pipeline still owns frozen points and persisted metadata.
Review order
build/configuration/build_algorithm.rs.builder/build.rs.builder/build/pipnn.rs.storage/bin.rsgraph validation and serialization.Validation
Stack
Stack 4/6. Depends on #1290. #1294 adds benchmark routes.